Arbitrary alphabet support #18
Conversation
|
Regarding one of your TODOs, I was thinking it might be a good idea to make a subpackage |
|
Also, |
|
The sub-packages are a bit of a nuisance when you fork the code. In my
fork, I have to change import statements from "
github.com/capitalone/fpe/fpeUtils" to "github.com/martinwaite/fpe/fpeUtils"
- and then remember to change them back in the PR. (In practice I use a
symlink from github.com/capitalone to github.com/martinwaite - but still a
nuisance.)
Given the small size of the packages, could we fold the sub-packages into
fpe ? We would need distinct `NewCipher` functions - `NewCipherFF1` and
`NewCipherFF3` ?
Some benefits of this are that we wouldn't need to export the utils
functions, and there are some other areas of code duplication that would be
easier to address in a single package.
The downside is that this would be a breaking change, and client code using
ff1 only would also build in ff3 code.
In the meantime, I will follow your suggestion.
…On Mon, 24 Dec 2018 at 21:24, Anit Gandhi ***@***.***> wrote:
Regarding one of your TODOs, I was thinking it might be a good idea to
make a subpackage fpeUtil and put the Codec and other new logic there.
Then ff1 and ff3 packages can import fpeUtil
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#18 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ACbnz3t45EXjgLtxduosWAfYxIfRI9AYks5u8UYMgaJpZM4ZeUBr>
.
|
|
That nuisance is present for any Go package, arguably. But I see your point - generally you should be cloning the repo into At the moment I'm not comfortable with merging everything into one giant package because like you said, it would be a breaking change. The addition of arbitrary alphabet support is awesome, but in my opinion it doesn't quite constitute making a v2 of this package. I don't really see an issue with exporting the utils functions, if anything, they might be useful to other folks. That said, you can make a Regarding the other points of code de-duplication, perhaps that can also be moved to fpeUtils? When I wrote the original code, I did note to myself that there are definitely places where the ff1 and ff3 code overlaps, but there were enough place where they diverged that I felt I could just keep them separate. |
|
btw @martinwaite I updated master branch with some new travis-ci config to do linting checks and bump the Go version, along with making a Go module. When you get a chance, please sync up your branch and if there are any linting issues, please fix. I noticed a few snake_case variables, for example. |
travis changes
|
@anitgandhi - I have integrated your Travis changes and made sure my additions pass the lint checks. I have removed 2 to-do tasks: eliminating the temporary array in DecodeNum would be difficult and not worth the effort; and using a map to translate from rune to numeral is OK. I think this is ready to merge in now. |
|
Thank you @martinwaite . I'll do a full code review of this when I get some time, hopefully this week. There are a lot of changes so it may take some time. I'll do my best to at least send some feedback in so you can incorporate it into changes iteratively. |
|
|
|
@anitgandhi curious to any reason this is not merged yet? |
|
The honest answer is because I was/am being a terrible maintainer. I sincerely apologize for that, especially to @martinwaite who put in the effort of tackling this long-standing enhancement. Unfortunately, I myself no longer have write access to this repository since I'm no longer at Capital One. I'm glad to see there is still interest in this package, though. I'm hoping I can get some level of access back so I can merge this change and some other bug fixes 🤞 |
What's in this PR?
This PR extends the radix range supported by the FF1 and FF3 packages from 62 to 65536.
It also adds support for alphabets containing utf-8 characters. Unicode support is basic - no attempt is made to deal with combining characters.
Backwards compatibility is maintained with the NewCipher function. In order to use an extended alphabet, use the NewCipherWithAlphabet instead.
While testing larger alphabets, the validation for radix based on the relation
exp(log(radix) * minlen) >= 100had a clause that rejected minlen < 2. That clause rejected alphabets longer than 100. In order to support longer alphabets, that clause has been removed - but perhaps we should set minlen to 2 in these cases.TODOs
This PR needs to be reviewed.
Housekeeping Items